Clearer error for SDPA when explicitely requested #28006
Conversation
fxmarty
requested review from
ArthurZucker,
amyeroberts and
LysandreJik
and removed request for
ArthurZucker
src/transformers/modeling_utils.py
Outdated
| f"{cls.__name__} does not support an attention implementation through torch.nn.functional.scaled_dot_product_attention yet. Please open an issue on GitHub to " | ||
| "request support for this architecture: https://github.com/huggingface/transformers/issues/new" | ||
| f"{cls.__name__} does not support an attention implementation through torch.nn.functional.scaled_dot_product_attention yet. Please request the" | ||
| ' support for this architecture: https://github.com/huggingface/transformers/issues/28005. If you believe this error is a bug, please open an issue in Transformers GitHub repository and load your model with the argument `attn_implementation="eager"` meanwhile.' |
There was a problem hiding this comment.
Wondering if we should include a specific code fix in the error message here as it's easy to get out-of-sync with the actual code / currently recommended fix
There was a problem hiding this comment.
Ah, sorry, I wasn't clear. I meant I don't think it's advisable to have and load your model with the argument attn_implementation="eager" meanwhile aa this suggestion might not always be correct
There was a problem hiding this comment.
Hum as of transformers==4.36 this would always work. #28003 needs to be properly fixed anyway. I'll think about it but having a clearer error message to easy fix is helpful in the short term I think
There was a problem hiding this comment.
I agree that it's useful in the short term, the point is more that it's hard to maintain in the long term. It's easy to forget that this is here, we don't go back to update and users default to a fix which is no longer recommended without us knowing.
Ultimately, it's just an error message. I don't feel super strongly about this. If you do think the fix is better to have I'm happy for it to be merged as-is.
|
@fxmarty Are you happy for me to merge? |
|
sure @amyeroberts ! |
* clearer error for sdpa * better message
* clearer error for sdpa * better message
As per title, partially fixes #28003.